fix(aws-s3-deployment): fix server side encryption parameters #6006

mattsains · 2020-01-28T23:17:08Z

I wasn't able to attach my local copy of the CDK to a package using one to perform an integration test, but I am happy to accept assistance doing this.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mattsains · 2020-01-28T23:21:36Z

In https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#step-4-commit

"Please follow the PR checklist written below", but I couldn't find anything like that in that file.
"assign the PR for a review to the "awslabs/aws-cdk" team.", but I could not add such a user from the Reviewers section of the PR UI

aws-cdk-automation · 2020-01-28T23:33:38Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-29T00:30:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-29T01:02:29Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

iliapolo

Hey @mattsains, PR looks good!

I do have a small note though. Like you pointed out, you changed the key names to match the aws s3 sync command line options as specified here: https://docs.aws.amazon.com/cli/latest/reference/s3/sync.html

Looking at the code, I see we actually have additional keys that don't match the CLI options:

website-redirect-location should be website-redirect
sse-customer-algorithm should be sse-c-copy-source

How do you feel about piggy backing this PR and issue to also fix those keys?

iliapolo · 2020-02-03T22:20:56Z

packages/@aws-cdk/aws-s3-deployment/test/test.bucket-deployment.ts

@@ -239,7 +239,8 @@ export = {
      contentLanguage: "en",
      storageClass: s3deploy.StorageClass.INTELLIGENT_TIERING,
      contentDisposition: "inline",
-      serverSideEncryption: s3deploy.ServerSideEncryption.AES_256,
+      serverSideEncryption: s3deploy.ServerSideEncryption.AWS_KMS,


This kind of removes our test for the AES_256 use-case, which we don't want to give up (correct me if i'm missing something here).
Lets just add another test for the AWS_KMS case. Which I see is completely missing from this file.

In general, we always prefer adding tests, rather than extending or changing existing ones. Even if it means duplicating most of the test code.

Your philosophy might vary, but this is just checking that the code correctly translates the property keys. The actual values of these properties doesn't have any factor on the test. I just changed this to AWS_KMS so I could add an additional property to the assertion. I could have left it as AES_256 and although it doesn't really make sense to specify a KMS key ID along with AES_256, the test would still pass.

Another thing to consider is what use case this test is testing: "object metadata can be given" - I haven't eroded this.

Your right, I didn't notice which exact test this was in. This test simply validates the given metadata: { "A": "1", "b": "2" } is translated into { 'x-amzn-meta-a': '1', 'x-amzn-meta-b': '2' }.

This means your added properties don't belong there (in fact none of the additional properties belong there).

What I would expect to see is an another test, perhaps call it 'props translate to system metadata', that validates the properties are translated to the expected keys. This test can than porbably be modified every time we add a property. Its important also not to mix test-cases.

I'm sorry, I don't understand this piece of feedback. It seems like you're asserting that this case is testing the metadata -> UserMetadata mapping. However, there are far more fields in this test for everything else -> SystemMetadata in this test.

Anyway, I'll copy this test case to another test case and remove most of the fields from the original test.

Sorry, I just realized the props translate to system metadata test im suggesting is in fact just a subset of the object metadata can be given test. Its just confusing because metadata is the name of property as well.

I'm still not liking the removal of that property value because in affect it does erode a test. Imagine for some reason we change this enum value, some test should fail. Currently, it looks like its this one.

Ok, now the confusion if out of the way, hopefully.

What i would do is:

Leave the current test as is.

Add another test that simply validates translation of the missing keys. Why another test? because I do want to use the AWS_KMS value in the new one, but also leave the test for AES_256.

Or whatever combination that gets us to a point we are validating both enum values.

Does that make sense?

This is a systemic problem with these tests. The following classes have values which are not tested:

CacheControl

StorageClass

For the purposes of getting this PR approved, I will add tests for the ServerSideEncryption enum

Sounds good

I updated this PR with that test.

iliapolo · 2020-02-03T22:33:18Z

@mattsains Wrote:

In https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#step-4-commit

"Please follow the PR checklist written below", but I couldn't find anything like that in that file.

"assign the PR for a review to the "awslabs/aws-cdk" team.", but I could not add such a user from the Reviewers section of the PR UI

Regarding the checklist, here it is: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#pull-request-checklist

Regarding the team, your'e right, its probably outdated. I'll see that its fixed. Thanks.

mattsains · 2020-02-03T22:40:02Z

I do have a small note though. Like you pointed out, you changed the key names to match the aws s3 sync command line options as specified here: https://docs.aws.amazon.com/cli/latest/reference/s3/sync.html

Looking at the code, I see we actually have additional keys that don't match the CLI options:
* _website-redirect-location_ **should be** _website-redirect_

* _sse-customer-algorithm_ **should be** sse-c-copy-source
How do you feel about piggy backing this PR and issue to also fix those keys?

Will do

iliapolo · 2020-02-03T22:41:42Z

@mattsains Wrote:

I wasn't able to attach my local copy of the CDK to a package using one to perform an integration test, but I am happy to accept assistance doing this.

Not sure what you mean by "attach my local copy...". Have you checked out our integration tests section? https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#integration-tests?

Also - probably more relevant to your use case, since you actually need to run cdk deploy to test everything works: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#running-cli-integration-tests.

Adding this test won't be a blocker for this PR, since i feel pretty confident about the fix and about our ability to maintain it. However, i'd still take a stab at getting a full integration test working as it will really help with future contributions, totally up to you :). I'm ready to help of course.

mattsains · 2020-02-03T22:51:58Z

While trying to fix the customer-algorithm property, I found another bug: #6080

I don't have time to fix that so I just created the above issue.

iliapolo · 2020-02-03T23:01:34Z

While trying to fix the customer-algorithm property, I found another bug: #6080

I don't have time to fix that so I just created the above issue.

Nice, thanks 👍

mattsains · 2020-02-03T23:04:59Z

I updated my branch with the requested changes.

Pull request has been modified.

iliapolo · 2020-02-03T23:15:55Z

packages/@aws-cdk/aws-s3-deployment/test/test.bucket-deployment.ts

@@ -239,7 +239,8 @@ export = {
      contentLanguage: "en",
      storageClass: s3deploy.StorageClass.INTELLIGENT_TIERING,
      contentDisposition: "inline",
-      serverSideEncryption: s3deploy.ServerSideEncryption.AES_256,
+      serverSideEncryption: s3deploy.ServerSideEncryption.AWS_KMS,


Your right, I didn't notice which exact test this was in. This test simply validates the given metadata: { "A": "1", "b": "2" } is translated into { 'x-amzn-meta-a': '1', 'x-amzn-meta-b': '2' }.

This means your added properties don't belong there (in fact none of the additional properties belong there).

What I would expect to see is an another test, perhaps call it 'props translate to system metadata', that validates the properties are translated to the expected keys. This test can than porbably be modified every time we add a property. Its important also not to mix test-cases.

Pull request has been modified.

aws-cdk-automation · 2020-02-03T23:43:28Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

fixes #6002

aws-cdk-automation · 2020-02-04T00:18:10Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-02-04T00:24:39Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Pull request has been modified.

aws-cdk-automation · 2020-02-04T01:03:59Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-02-04T08:27:21Z

Thank you for contributing! Your pull request is now being automatically merged.

mattsains requested a review from a team January 28, 2020 23:23

skinny85 assigned skinny85 and iliapolo and unassigned skinny85 Jan 29, 2020

mattsains requested a review from iliapolo January 31, 2020 19:43

iliapolo previously requested changes Feb 3, 2020

View reviewed changes

fix(aws-s3-deployment): fix server side encryption parameters

fc8897d

fixes #6002

iliapolo previously approved these changes Feb 4, 2020

View reviewed changes

mattsains removed the request for review from a team February 4, 2020 00:02

Merge branch 'master' into master

b097e1c

mattsains requested a review from iliapolo February 4, 2020 00:26

iliapolo approved these changes Feb 4, 2020

View reviewed changes

mergify bot merged commit c7197c0 into aws:master Feb 4, 2020

iliapolo mentioned this pull request Feb 4, 2020

fix(s3-deployment): passing any system metadata causes lambda to fail on "Unknown options:" when invoking aws cli. #6086

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(aws-s3-deployment): fix server side encryption parameters #6006

fix(aws-s3-deployment): fix server side encryption parameters #6006

mattsains commented Jan 28, 2020

mattsains commented Jan 28, 2020

aws-cdk-automation commented Jan 28, 2020

aws-cdk-automation commented Jan 29, 2020

aws-cdk-automation commented Jan 29, 2020

iliapolo left a comment

iliapolo Feb 3, 2020

mattsains Feb 3, 2020

mattsains Feb 3, 2020

iliapolo Feb 3, 2020

mattsains Feb 3, 2020

iliapolo Feb 3, 2020

iliapolo Feb 3, 2020 •

edited

Loading

mattsains Feb 3, 2020

iliapolo Feb 3, 2020

mattsains Feb 3, 2020

iliapolo commented Feb 3, 2020 •

edited

Loading

mattsains commented Feb 3, 2020

iliapolo commented Feb 3, 2020 •

edited

Loading

mattsains commented Feb 3, 2020

iliapolo commented Feb 3, 2020

mattsains commented Feb 3, 2020

iliapolo Feb 3, 2020

aws-cdk-automation commented Feb 3, 2020

aws-cdk-automation commented Feb 4, 2020

aws-cdk-automation commented Feb 4, 2020

aws-cdk-automation commented Feb 4, 2020

mergify bot commented Feb 4, 2020

fix(aws-s3-deployment): fix server side encryption parameters #6006

fix(aws-s3-deployment): fix server side encryption parameters #6006

Conversation

mattsains commented Jan 28, 2020

mattsains commented Jan 28, 2020

aws-cdk-automation commented Jan 28, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 29, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 29, 2020

AWS CodeBuild CI Report

iliapolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iliapolo Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iliapolo commented Feb 3, 2020 • edited Loading

mattsains commented Feb 3, 2020

iliapolo commented Feb 3, 2020 • edited Loading

mattsains commented Feb 3, 2020

iliapolo commented Feb 3, 2020

mattsains commented Feb 3, 2020

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 3, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 4, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 4, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 4, 2020

AWS CodeBuild CI Report

mergify bot commented Feb 4, 2020

iliapolo Feb 3, 2020 •

edited

Loading

iliapolo commented Feb 3, 2020 •

edited

Loading

iliapolo commented Feb 3, 2020 •

edited

Loading